Skip to content

Fix PointConverter's leading white-space handling#130692

Open
enyim wants to merge 1 commit into
dotnet:mainfrom
enyim:enyim/convert-point-leading-whitespace
Open

Fix PointConverter's leading white-space handling#130692
enyim wants to merge 1 commit into
dotnet:mainfrom
enyim:enyim/convert-point-leading-whitespace

Conversation

@enyim

@enyim enyim commented Jul 14, 2026

Copy link
Copy Markdown

Allow PointConverter to convert string values that have leading white-space. Aligns behavior with .NET Framework 4.x

Test have been updated to include an extra space.

Fixes #130690

@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Jul 14, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@enyim

enyim commented Jul 14, 2026

Copy link
Copy Markdown
Author

@dotnet-policy-service agree

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-componentmodel
See info in area-owners.md if you want to be subscribed.

@jeffhandley jeffhandley left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

This review was generated by the holistic code review workflow being iterated on as part of #130339. Please treat its findings as assistive input for human review.

Holistic Review

Motivation: The compatibility problem is real: PointConverter trims the input before splitting, so leading whitespace makes the split ranges relative to a different span than the original string that was being sliced. Matching .NET Framework behavior for WinForms/resx data with leading whitespace is justified.

Approach: For PointConverter, slicing the same trimmed span that produced the Range values is the right targeted fix and preserves the existing culture-specific separator and number conversion behavior. However, the same parsing pattern exists in sibling drawing converters, so the root cause appears only partially addressed.

Summary: ⚠️ Needs Changes. The PointConverter code change itself looks correct, and no new public API surface is introduced. Before merging, I think the same trimmed-span/range mismatch should be fixed and tested in the sibling geometric converters (RectangleConverter, SizeConverter, and SizeFConverter) or explicitly acknowledged as a deliberately deferred follow-up by the area owners.


Detailed Findings

✅ Correctness — PointConverter now slices the same span it split

In src/libraries/System.ComponentModel.TypeConverter/src/System/Drawing/PointConverter.cs lines 29-48, the ranges returned by text.Split(...) are now applied back to text rather than to strValue. That fixes the leading-whitespace failure because the range coordinates are relative to the trimmed span, while keeping null/empty handling, culture-specific list separators, trailing whitespace tolerance, and int conversion semantics unchanged.

❌ Cross-cutting completeness — sibling converters still have the same bug

The same pattern remains in the other geometric converters:

  • src/libraries/System.ComponentModel.TypeConverter/src/System/Drawing/RectangleConverter.cs lines 29-50 trims into text, splits text, then slices strValue[ranges[i]].
  • src/libraries/System.ComponentModel.TypeConverter/src/System/Drawing/SizeConverter.cs lines 29-48 does the same for Size.
  • src/libraries/System.ComponentModel.TypeConverter/src/System/Drawing/SizeFConverter.cs lines 29-48 does the same for SizeF.

Those ranges are also relative to the trimmed span, so inputs like " 1, 2", " 1, 2, 3, 4", or culture-equivalent values will still parse the wrong substring when the original string has leading whitespace. Recent file history shows these converters were refactored together in 350b9cb5438 Refactor geometric type converters for improved parsing (#111349), which is consistent with this being the same regression. Given the PR is fixing .NET Framework-compatible leading-whitespace handling, I would update the sibling converters in the same PR (or have maintainers explicitly decide to defer them with a tracking issue).

⚠️ Test Quality — coverage is narrow to PointConverter

src/libraries/System.ComponentModel.TypeConverter/tests/Drawing/PointConverterTests.cs lines 233-241 now covers a leading-whitespace ConvertFromString input for PointConverter, which is useful and should fail without the production fix. If the sibling converters are fixed here, please add matching regression coverage in RectangleConverterTests, SizeConverterTests, and SizeFConverterTests as well so the shared parsing bug cannot recur there.

@github-actions

github-actions Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "a44a45573f5846dd63653375a1b2370e6bf8d0ed",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "ac90ce8e7a5c217d4ab9effb35c82e5b6596371f",
  "last_reviewed_commit": "a44a45573f5846dd63653375a1b2370e6bf8d0ed",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "ac90ce8e7a5c217d4ab9effb35c82e5b6596371f",
  "last_recorded_worker_run_id": "29685662377",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "a44a45573f5846dd63653375a1b2370e6bf8d0ed",
      "review_id": 4730701277
    }
  ]
}

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holistic Review

Motivation: PointConverter.ConvertFrom computes split ranges from the trimmed span (text = strValue.AsSpan().Trim()), but then indexed those ranges back into the untrimmed strValue. When the input string had leading (or trailing) whitespace, the range offsets were shifted relative to strValue, so the extracted x/y substrings were wrong and parsing failed. This diverged from .NET Framework 4.x behavior. Fixes #130690.

Approach: The two ConvertFromString calls now index into the trimmed text span instead of strValue (text[ranges[0]].ToString() / text[ranges[1]].ToString()), matching the source of the ranges produced by text.Split(...). The existing ConvertFromString theory test was updated to prepend a leading space so the whitespace path is exercised.

Summary: The fix is correct, minimal, and well-targeted: it makes the range indexing consistent with the span the ranges were derived from, which is the root cause of the bug. The test change meaningfully covers the previously-broken leading-whitespace case (and, since the error message on line 43 already used text.ToString(), the fix also keeps parsing and error reporting consistent). No new allocations are introduced beyond the pre-existing .ToString() per component.

Two non-blocking observations (both pre-existing and out of scope for this PR, not requiring action here):

  • Sibling converters in the same area (e.g. SizeConverter, RectangleConverter, PointFConverter) historically shared this trimming/splitting pattern; if any still index a non-trimmed string with trimmed-span ranges they would have the same latent bug. Worth a follow-up glance, but nothing in this PR needs to change.
  • The unsafe keyword on ConvertFrom appears unused in the current body; also pre-existing and unrelated.

Verdict: LGTM. The change is a clean, correctly-scoped bug fix with appropriate test coverage.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 41.3 AIC · ⌖ 10.4 AIC · ⊞ 10K

@enyim

enyim commented Jul 19, 2026

Copy link
Copy Markdown
Author

do you want fixes for the rest of the converters, and if yes, in this PR or separate (separate per converter or just a single)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.ComponentModel community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

System.Drawing.PointConverter cannot convert values with leading white-space

2 participants